This pull request has been automatically marked as stale because it has not had recent activity for 21 days.

To keep this PR open, please:

• Rebase on the latest develop branch
• Address any pending review comments
• Reply with an update on the PR status

If no further activity occurs within the next 14 days, this PR will be automatically closed.

This pull request has been automatically closed due to inactivity.

If you'd like to continue working on this:

1. Reopen this PR or create a new one
2. Rebase on the latest develop branch
3. Address any pending review comments

Thank you for your contributions to POS Next!

What to finish before re-review

Everything open from review #5142106656, as a checklist. Each item names the file it lives in and the condition that closes it, so "done" is demonstrable rather than asserted.

21 items — 5 must close before re-review (Gate 1), 7 correctness, 4 tests, 3 cleanup, 2 hygiene.

Gate 1 — Blocking

The validation layer here is thorough and each validator is tested. These five are where the money actually escapes it.

• B1 — Make a blank expense maximum mean "not configured", not "unlimited" · custom/pos_profile.json:342, api/expenses.py:134
posa_maximum_expense_amount ships with no default (every other new field in that file carries default: "0"), and validate_expense_amount does if maximum_amount <= 0: return. Tick Allow POS Expense, get an empty Currency field, and the only monetary bound on the endpoint is gone. From there any cashier with an open shift can submit unbounded Journal Entries crediting the till through a whitelisted endpoint running ignore_permissions=True.
  Done when: enabling POS expenses without setting a maximum either refuses the expense or applies a shipped default — and the field description says what blank means.

• H1 — Lock the shift around check-and-insert · api/expenses.py:127, 311
validate_expense_amount reads the shift total, compares, returns; _create_expense_journal_entry inserts afterwards, with nothing serialising the two. Two concurrent requests each pass a 100 limit with amount 100 and both commit. The dialog's disabled-while-submitting button closes the casual double-click, so the open path is a replayed or scripted request.
  Done when: the total is read under for_update=True on the opening shift, inside the same transaction as the insert.

• H2 — Compute the limit from the credit rows, not posa_expense_amount · api/expenses.py:154
  That custom field is independent of the JE's actual lines and is read_only: 1 in the UI only — still writable via API. A JE with posa_is_pos_expense = 1 and a negative posa_expense_amount lowers the shift total and inflates everyone's allowance. JE create is privileged, which caps severity, but summing the credits removes the class of problem.
  Done when: get_shift_expense_total derives from the JE lines, and a JE whose custom field disagrees with its rows can't move the limit.

• H3 — Make the two ignore_permissions bypasses a recorded decision · api/expenses.py:273, 307
get_active_employees returns 200 employee IDs and names; get_expense_accounts returns the company's entire expense chart of accounts with limit_page_length=0. Both reachable by anyone with an open shift. The docstring explains why ("POS cashiers may lack Employee read perm") — that may be the right trade, but it's documented in one docstring and nowhere else.
  Done when: the bypass is justified in the PR description, the account list is bounded or server-searched, and the employee list returns only what the dialog needs.

• H4 — Add a cancel path · api/expenses.py:368, pos_closing_shift.py
create_pos_expense submits with no way to reverse it, so a mistyped amount needs desk access. Worse: get_pos_expenses filters docstatus = 1, so cancelling a JE after the shift closes silently drops it from that query while the submitted POS Closing Shift keeps the old total_pos_expenses and reconciliation rows. The two records disagree permanently, with nothing flagging it.
  Done when: a cancel endpoint exists with the same shift/ownership checks as create, and it refuses once the shift is closed.

Gate 2 — Correctness

• M1 — Scope the report by company; fix or drop the cashier role · pos_expense_report.py:96, pos_expense_report.json:36
get_data builds its WHERE from supplied filters only — company is optional, and raw SQL bypasses Company User Permission records. Any role holding Journal Entry report permission (including Sales Manager) sees every company. Separately, the POSNext Cashier grant is inert: query_report.run also checks has_permission("Journal Entry", "report") (frappe/desk/query_report.py:207) and that role has no JE permission, so the menu entry appears for people it will refuse.
  Done when: company is mandatory or defaulted and the user's permitted companies are applied; and the cashier either gets the permission or is dropped from the roles list.

• M2 — Make the closing-shift expense row an audit record · pos_closing_shift_expense.json:16
expense_account and employee are Data not Link, and there is no journal_entry field. No click-through to the voucher, no referential integrity, and nothing tying total_pos_expenses to the JEs behind it. get_pos_expenses already returns journal_entry; make_closing_shift_from_opening just drops it.
  Done when: the row links to its Journal Entry, and account/employee are Links.

• M3 — Stop resolving the payment account to an arbitrary ledger · api/expenses.py:249 → api/invoices.py:294
get_payment_account's fifth fallback is frappe.db.get_value("Account", {...Cash/Bank...}) with no order_by. It was written for invoice payment display; this PR is the first caller to use its result as the credit side of a submitted JE. Also here: the bare except Exception: return None swallows a specific, actionable error and replaces it with "Payment Account is required", and the resolver runs twice per create.
  Done when: an unresolvable payment account fails loudly with the underlying reason, and no expense can credit a ledger nobody chose.

• M4 — Decide whether non-cash modes should be allowed · api/expenses.py:204
validate_mode_of_payment checks the mode is on the profile and resolves to an account, but not what kind. An expense against a card mode credits the bank account, and make_closing_shift_from_opening then subtracts it from expected card takings.
  Decide: if paying by card is intended, say so in the field help; if not, restrict to account_type = "Cash".

• M5 — Post overnight expenses to the shift's date · api/expenses.py:331
posting_date: today(). A shift opened at 22:00 with an expense at 00:30 posts to the new day while reconciliation still counts it against the shift. Drawer maths holds; GL date and shift date diverge.
  Done when: the posting date derives from the shift, or the divergence is a documented decision.

• M6 — Stop overloading cheque_no · api/expenses.py:334
  The shift is already in posa_pos_opening_shift a few lines below. cheque_no / cheque_date are the JE's bank-reference fields and surface in bank reconciliation views.

• M7 — Handle multi-currency on the Journal Entry · api/expenses.py:350
  Only *_in_account_currency is set — no exchange_rate, no company-currency debit/credit. On a foreign-currency payment account ERPNext fills those from today's rate. The rest of this app takes this seriously; pos_closing_shift.py has a whole get_base_value helper for it.

Gate 3 — Tests

17/17 pass and every validator has a real assertion — this is genuinely good unit testing. The gap is that nothing exercises the part that moves money.

• T1 — One test that builds a real Journal Entry · new
  _create_expense_journal_entry is mocked out in the only test that calls create_pos_expense (test_expenses.py:183-211), so nothing verifies that debit and credit balance, that cost_center lands on both rows, M3's account resolution, M5's posting date, or M7's currency handling. One integration test covers all five.

• T2 — Cover the "no maximum" behaviour · test_expenses.py
  A test asserting what happens when posa_maximum_expense_amount is unset would have made B1 obvious at review time. Add it, then change it when B1 is fixed.

• T3 — Replace the two change-detector tests · test_expenses.py:159, 213
  Both assert full assert_called_once_with argument lists, so reordering a fields list breaks them with no bug present. The one at :159 also asserts the permission bypass from H3, which makes that harder to revisit.

• T4 — Cover the reconciliation seam · pos_closing_shift.py
  Nothing tests make_closing_shift_from_opening's expense aggregation — where expenses meet payment_reconciliation and total_pos_expenses. That is also where H4's desync shows up.

  Not asked for, but noted: there are no .spec.js files anywhere on this branch, so ExpenseDialog.vue (363 lines, carrying the client-side limit arithmetic) has no frontend test. The vitest harness arrives in Feature/thin develop split #360 — worth adding one once these land in the same branch.

Gate 4 — Cleanups

• L1 — <ExpenseDialog> is mounted without an @expense-created listener, but the component emits it; the emit is dead and nothing refreshes after recording · POSSale.vue:590
• L2 — currency prop defaults to "USD" in a multi-currency POS; require it, or default to the profile currency · ExpenseDialog.vue:146
• L3 — get_expense_accounts uses limit_page_length=0; the dialog already has a search box, so a server-side search query is the natural shape · api/expenses.py:307

Gate 5 — Before asking for re-review

• D1 — Rebase on develop. This branch is 17 commits behind (merge base 9212350) and was last touched 2026-09-04. Rebase so the diff under discussion is the diff that will merge.
• D2 — Coordinate the collision with Feature/thin develop split #360. Both PRs are open against develop and both touch POS/components.d.ts, POS/src/components/sale/InvoiceCart.vue, POS/src/pages/POSSale.vue and pos_next/api/bootstrap.py. components.d.ts will conflict outright — both register a new component. Agree which merges first, and re-check that both features still appear in the POS menu afterwards.

Definition of done

bench --site <site> run-tests --app pos_next --module pos_next.api.test_expenses
ruff check pos_next/api/expenses.py pos_next/pos_next/report/pos_expense_report/

# the one that proves B1 is fixed:
# enable posa_allow_pos_expense on a profile, leave the maximum blank,
# call create_pos_expense -> must refuse, not succeed

Current baseline for comparison: 17/17 pass, ruff clean. Both were verified on a local bench before this review.

Cross-reference: this PR and #360

Both are open against develop and both touch the same four files, so they need sequencing rather than parallel merges.

Shared files

components.d.ts will conflict outright — each registers a new component. The other three are textually adjacent rather than contradictory, but all four want a human resolving them.

Suggested order: this PR first.

Not a judgement on quality — it is about how far each one's blocker is from done:

• Pn 65 expenses in pos screen #318 (this PR): the blocker is a config decision — posa_maximum_expense_amount ships with no default and <= 0 is read as "unlimited". Five Gate 1 items, all inside api/expenses.py and one JSON file. Nothing here depends on Feature/thin develop split #360.
• Feature/thin develop split #360: the blocker is architectural — the optional-app gate is evaluated once per worker process against an arbitrary site, so the split's central mechanism does not currently engage. That is a rework, plus two open ownership decisions.

Landing this one first means #360 absorbs the four-file conflict, which is fair: it is the PR staying open longer, and it is already rebasing.

Whoever merges second: resolve all four by hand, then confirm both the POS Expense and the Return authorization entries still appear in the POS menu — the two features add sibling buttons to the same menu and the same dialog mount point, so a bad merge silently drops one.

Both PRs also need a rebase regardless — this one is 17 commits behind develop, #360 is behind its own merge base a07f981.

Reviews and per-PR checklists are in the comments on each. Item counts: 21 here, 29 on #360.

Review board — PRs #318 and #360

Everything open across both PRs, in one place. 50 items. Full detail is in each PR's review and checklist comment; this is the consolidated view.

Merge sequence

First — land #318. Its blocker is a one-line config decision. Five Gate 1 items, all local to one module. Nothing in it depends on #360.

Then — rebase #360 onto it. #360 resolves the four-file collision, since it is the one still needing architectural work. Merging it first would make #318 pay a cost it did not incur.

Throughout — neither ships without its blocker closed. Both blockers are unbounded-authority defects, not polish. Everything else on this board can follow.

The two blockers

Everything else here is secondary to these.

#318 — A blank expense maximum means unlimited

Tick one checkbox on a POS Profile and every cashier on it can submit unbounded Journal Entries crediting the till, through a whitelisted endpoint running ignore_permissions=True. posa_maximum_expense_amount ships with no default, and <= 0 is read as "no limit".

custom/pos_profile.json:342 · api/expenses.py:134

#360 — The optional-app gate never engages

Decided once per worker process against whichever site it touched first. Reproduced on a 3-site bench: fleet.localhost, which does not have pos_next installed, decided pos_next's pricing-patch state for posnext.local, which does. The split's central mechanism does not do what it claims.

pos_next/__init__.py:27

#318 · POS expenses — 21 items

Closer to mergeable. Validators are thorough and tested; 17/17 pass. The gaps are all downstream of the money leaving.

Gate 1 — Blocking (5)

• B1 Blank maximum must mean "not configured", not unlimited · custom/pos_profile.json:342, api/expenses.py:134
• H1 Lock the shift around check-and-insert · api/expenses.py:127
• H2 Sum the credit rows, not the writable custom field · api/expenses.py:154
• H3 Justify and narrow the two ignore_permissions bypasses · api/expenses.py:273, 307
• H4 Add a cancel path, refused once the shift closes · api/expenses.py:368

Gate 2 — Correctness (7)

• M1 Scope the report by company; fix or drop the inert cashier role · pos_expense_report.py:96
• M2 Link the closing-shift row to its Journal Entry; Data → Link · pos_closing_shift_expense.json
• M3 Stop resolving the payment account to an arbitrary ledger · api/expenses.py:249
• M4 🔀 decide — whether non-cash modes may fund an expense · api/expenses.py:204
• M5 Post overnight expenses to the shift's date · api/expenses.py:331
• M6 Stop overloading cheque_no to carry the shift · api/expenses.py:334
• M7 Handle multi-currency on the Journal Entry · api/expenses.py:350

Gate 3 — Tests (4)

• T1 One test that builds a real Journal Entry — covers M3, M5, M7 at once
• T2 Cover the "no maximum" behaviour that hid B1
• T3 Replace the two change-detector tests · test_expenses.py:159, 213
• T4 Cover the reconciliation seam in make_closing_shift_from_opening

Gate 4–5 — Cleanup & hygiene (5)

• L1 Dead @expense-created emit — nothing refreshes · POSSale.vue:590
• L2 Hardcoded "USD" currency default · ExpenseDialog.vue:146
• L3 Unbounded account list — server-side search instead · api/expenses.py:307
• D1 Rebase — 17 commits behind develop
• D2 Coordinate the four-file collision with Feature/thin develop split #360

#360 · Thin develop split — 29 items

Further out. The auth gate underneath is well built; the problems are at its edges and in the split mechanism itself.

Gate 1 — Blocking (6)

• B1 Move the optional-app gate to call time; delete the duplicate helper · pos_next/__init__.py:27
• H2 TTL and invalidation on the wallet-mode cache · api/wallet.py:182
• H3 Stop OR-ing company and transaction currency · overrides/sales_invoice.py:124
• H4 Show the approver the action, amount and invoice · AuthorizationDialog.vue:15
• H5 Key the rate limit on approver, not IP · api/authorization.py:95
• M1 Close self-service PIN enrolment · api/authorization.py:163

Gate 2 — Correctness & decisions (7)

• M3 🔀 decide — who owns the authorization gate · POS/src/utils/promoApi.js:11
• M6 🔀 decide — move Gift Pool and GWP out of the core cart · posCart.js:1134–1330
• M2 Create the two audit fields, or stop writing to them · authorization/gate.py:102
• M4 Move payment-account validation after super().validate() · overrides/sales_invoice.py:76
• M5 Stop silently re-attributing invoices to the walk-in customer · overrides/sales_invoice.py:90
• M8 Wire enforce_context to a real action, or delete it · authorization/gate.py:64
• M9 pickle → json; stop the grant TTL refreshing itself · authorization/grants.py:40, 96

Gate 3 — Tests (5)

• T1 A test that would have caught B1
• T2 Cover overrides/sales_invoice.py — currently zero
• T3 Pin the GWP carve/restore round trip
• T4 Replace the smoke-test assertions that cannot fail
• T5 Prove the offline cart and server agree on gifts

Gate 4–5 — Cleanup & hygiene (11)

• L1 PIN removal logged as RESULT_PIN_SET · api/authorization.py:185
• L2 Unreachable branch in _resolve_leaf_territory · api/customers.py:229
• L3 Failing external sync rolls back a created customer · integrations/registry.py:61
• L4 Redundant sync_return_loyalty_program in on_submit
• L5 Smoke runner hardcodes site brainwise.dev
• L6 Note the memory ceiling on the Item Price snapshot
• L7 Clear ruff on the changed files
• L8 Confirm the deleted packed-bundle merge is covered
• D1 Write the PR description — body is empty
• D2 Document the fail-closed return behaviour and the 180s TTL
• D3 Rebase on develop

The collision

Both PRs touch POS/components.d.ts, POS/src/components/sale/InvoiceCart.vue, POS/src/pages/POSSale.vue and pos_next/api/bootstrap.py. The first will conflict outright — each registers a new component. Whoever merges second resolves all four by hand and re-checks that both features still appear in the POS menu.

Four decisions, not patches

Marked 🔀 above. Each wants a person, and each belongs in the PR description once settled.

1. Feature/thin develop split #360 — who owns the authorization gate (pos_next or the satellite).
2. Feature/thin develop split #360 — whether Gift Pool and GWP belong in the core cart, given offerStrategies.js exists for exactly that.
3. Pn 65 expenses in pos screen #318 — whether non-cash modes may fund an expense.
4. Pn 65 expenses in pos screen #318 — what a blank expense maximum should mean.

What I withdrew

Two findings did not survive checking, and are deliberately not on this board:

• Feature/thin develop split #360 — get_invoices. I reported it as a regression that lost search, date filters and the page-size clamp. Wrong: I diffed develop..pr360 instead of the merge base. Commit 98b39e5 improved it on develop after this branch point; the PR never touched it and the rebase will take develop's version.
• Pn 65 expenses in pos screen #318 — the report's POSNext Cashier grant. It looked like cross-company exposure. It isn't: frappe.desk.query_report.run also checks has_permission("Journal Entry", "report") (frappe/desk/query_report.py:207), and that role holds no Journal Entry permission on this bench, with none added by the shipped fixtures. A cashier gets refused. The inert role grant is filed as a config bug inside M1; the real half is the missing company scoping.

#318 @ 04034eb (merge base 9212350) · #360 @ 5d4c16f (merge base a07f981). Verified on a local bench: Frappe + ERPNext, 3 sites, dns_multitenant. #318 backend 17/17 pass; #360 vitest 12/12 pass; ruff and biome run on both.

D2 closed — collision with #360

Merge order (agreed): land #318 first, then rebase #360 onto develop.

Same rationale as the review board: #318’s remaining work is local; #360 is the longer-lived architectural PR and should absorb the four-file resolve.

Shared files — keep both sides

components.d.ts will conflict outright (both register a new component). Resolution is additive: both lines stay.

Post-merge menu re-check (owner of #360 after rebase)

1. POS Expense still in the header ⋮ menu (gated by canRecordPosExpense).
2. POS Expense still on the InvoiceCart empty-cart grid (gated by allowPosExpense).
3. Return Invoice still on that same grid — Feature/thin develop split #360 does not add a sibling menu entry; it gates the existing Return path via AuthorizationDialog mounted in App.vue.
4. Management slider still shows Promotions (and Product Management / Stock Lookup as on develop).

Correction to the earlier cross-ref wording: current #360 only adds :company in POSSale.vue; the auth dialog mount is in App.vue, not a second POSSale menu button. The failure mode to watch is still the same — a bad resolve that drops Expense wiring or leaves Return unwired to requireAuthorization.

Task list — after re-review at 7818730

Re-reviewed against merge base d02f852 (current develop tip). The 21 items from
review #5142106656
are 18 closed, 3 partial, 0 untouched. Test count went 17 → 26 and the rebase is fully current.

6 items open — 4 must close before merge (Gate 1), 2 cleanup. Same format as last time:
each names the file and the condition that closes it.

Gate 1 — Blocking

• N1 — POS Payment Method.default_account does not exist · api/expenses.py:383, :394
  M3's rewrite resolves a payment account in three steps. Steps 2 and 3 read default_account
  off POS Payment Method; ERPNext v15 declares only default, mode_of_payment,
  allow_in_returns, and pos_next adds no custom field. Both confirmed on a bench:

  step 2 (:383) → OperationalError (1054, "Unknown column 'default_account' in 'SELECT'")
  step 3 (:394) → OperationalError (1054, "Unknown column 'ppm.default_account' in 'SELECT'")
  

  Step 2 is the production path (validate_mode_of_payment always passes pos_profile). So the
  exact misconfiguration M3 asked to fail loudly now raises a raw SQL error, and the friendly
  frappe.throw at :401 is unreachable. This is also why T1 errors.
  Done when: steps 2 and 3 are deleted, leaving Mode of Payment Account → throw. Deletion is
  the right call, not just the smaller one — get_cash_payment_methods builds the dialog's mode
  list from pos_profile.get_payment_methods, which resolves account_type through a join on
  Mode of Payment Account alone. A mode with no row there never reaches the dialog, so these two
  steps can only ever fire on the error path they break.

• N6 — the amount the cashier types has no agreed currency · ExpenseDialog.vue:208 ↔ api/expenses.py:525
  H2 and M7 are each right and they agree with each other. The gap is at their shared boundary:
  nothing establishes that the typed number is in company currency.
  POSSale.vue:597 passes :currency="shiftStore.profileCurrency" → posShift.js:17 →
  POS Profile.currency, a reqd: 1 Link set independently of Company.default_currency.
  The dialog labels the limit, the recorded total, the remaining allowance and every listed
  expense with it — while the server books the same number as company currency and enforces the
  limit in company currency.
  On a profile whose currency differs from its company's, a cashier shown "Shift limit: 500" who
  types 100 gets 100 company currency debited, measured against 500 company currency. Both
  the GL amount and the bound are off by the exchange rate. Reachable only in the multi-currency
  setups M7 was written for. posa_maximum_expense_amount also carries options: null, so it
  declares no currency basis at all.
  Done when: the basis is stated and enforced one way. Simplest: label the dialog and the field
  in company currency (fetch Company.default_currency instead of profileCurrency, set the
  custom field's options). Converting the input instead is valid too — but then the limit
  comparison has to convert as well.

• N2 — the T1/T4 integration tests build no fixtures · test_expenses.py:398, test_pos_closing_shift.py:310
  Both new classes declare _Test Company, Travel Expenses - _TC, Main - _TC and
  _PNXT_TEST_POS_PROFILE__Test Company as bare constants and have only a tearDown that rolls
  back — no setUpClass, no record creation. That profile name is built in exactly one place in
  this repo: a local helper in pos_next/test_promotions.py:304, which neither module calls.
  So all four pass only if an unrelated module happened to run first and leave records behind. On
  a clean bench they error before reaching an assertion — which means T1 and T4 cannot yet
  demonstrate the money path or the reconciliation seam.
  Done when: each class creates its own company / accounts / cost center / POS Profile in
  setUpClass (or reuses the test_promotions.py helper), and both modules pass from a clean DB.

• N3 — unrelated work rides along in this branch
  Eight files, ~350 lines, nothing to do with expenses:

  Commit	Brings in	Files
  8231e26	PR feat: enhance offline batch and serial number management #359's offline batch/serial work — still open separately	BatchSerialDialog.vue, itemSearch.js, serialNumber.js, offline/items.js (+111)
  05a7b28 59acd2f 9e9c26f	EOD-report fixes + translations	pos_next_eod_report.json, pos_closing_print.py, test_pos_closing_print.py, 3 × .csv
  f5ae353	"track collected money alongside invoiced totals"	ShiftClosingDialog.vue, pos_closing_shift.py

  Three of feat: enhance offline batch and serial number management #359's four files are byte-identical to that PR's head (verified by hash). Whichever
  of Pn 65 expenses in pos screen #318 / feat: enhance offline batch and serial number management #359 lands second now conflicts on files neither review covered, and feat: enhance offline batch and serial number management #359's review no
  longer governs code shipping through Pn 65 expenses in pos screen #318.
  Done when: those commits are dropped and land through their own PRs — or the description says
  they are deliberately bundled and names who reviewed them.

Gate 2 — Cleanup

• N4 — ruff regression · test_pos_closing_shift.py:24
I001 unsorted-imports. Last review's baseline was ruff-clean on every changed file.
  ruff check --fix closes it.

• N5 — timer not cleared on unmount · ExpenseDialog.vue:414
accountSearchTimer is cleared on each keystroke but never on unmount, so a dialog closed within
  250 ms of typing still fires one search. Harmless — the resource call is guarded — but
  onUnmounted(() => clearTimeout(accountSearchTimer)) is one line.

Closed last round — no action

B1 H1 H2 H3 H4 · M1 M2 M4 M5 M6 M7 · T2 T3 · L1 L2 L3 D1 D2

M3, T1 and T4 are the three partials, tracked above as N1 and N2.

Definition of done

# both must pass from a clean database, not just a warm bench
bench --site <site> run-tests --app pos_next --module pos_next.api.test_expenses
bench --site <site> run-tests --app pos_next --module pos_next.pos_next.doctype.pos_closing_shift.test_pos_closing_shift

ruff check $(git diff --name-only $(git merge-base HEAD upstream/develop)..HEAD | grep '\.py$')

# the one that proves N1 is fixed: a Mode of Payment with no Mode of Payment Account row
# -> must throw "Please set default Cash account in Mode of Payment X", not OperationalError 1054

Current: 26 tests / 25 pass / 1 error (test_expenses), 11 / 8 / 3 error
(test_pos_closing_shift), 1 ruff error. Measured on Frappe 15.111.1 + ERPNext 15.111.0.

bench run-tests --app pos_next (whole app) cannot collect at all — test_bank_deposits.py:4
imports IntegrationTestCase, absent in Frappe v15. Pre-existing on develop, not this PR,
but it means per-module runs are the only option today.

Still open at 3ab88dc — two items

Re-checked against head 3ab88dc, merge base d02f852. N1, N2, N4, N5 are closed, and
M3 / T1 / T4 close with them. The gate is green on a live bench:

Two things are not done. Neither is a redesign.

1. N6 is not closed — the store rung it added is inert · POS/src/stores/posShift.js:19

640533d made the right call and recorded it in three places (field options, both
docstrings, the error formatting). Server side is correct. The client wiring is not:

const companyCurrency = computed(
    () => currentProfile.value?.company_currency || profileCurrency.value,
)

currentProfile is not the bootstrap payload. It is shiftState.value.pos_profile
(useShift.js:157), and every writer of shiftState (useShift.js:32, 50, 68, 100, 139)
is fed by shifts.check_opening_shift or shifts.create_opening_shift — both of which
return frappe.get_doc("POS Profile", …) (shifts.py:95, :142). A POS Profile document
has no company_currency field. The key bootstrap.py:89-99 adds lands in a different
store (stores/bootstrap.js:59) that nothing writes into shiftState.

So company_currency is always undefined, and companyCurrency always evaluates to
profileCurrency — POS Profile.currency, the exact value N6 said must not be the basis.
POSSale.vue:597 passes that into a prop documented as "Company.default_currency … not
POS Profile.currency."

Not user-visible today, and worth saying plainly: ExpenseDialog's currency prefers
dialogDataResource.data?.company_currency (:215-222), which is server-authoritative and
correct, and the whole form sits behind v-else on dialogDataResource.loading
(ExpenseDialog.vue:4 / :9) so no label paints before it resolves. The defect is that the
two fallback rungs behind it are wrong, under a name and a docstring that say they are
right — which is how this comes back later.

Done when: companyCurrency reads a source that actually carries it. shiftState.company
is already a full Company document from all three endpoints:

const companyCurrency = computed(
    () => shiftState.value.company?.default_currency || profileCurrency.value,
)

One line, and it covers the online path, the offline-restore path and create_opening_shift.

2. N3 — the branch split, and a4f6ffd made it bigger

PR description is unchanged, and the commits are still here. Neither half of the "done when"
happened.

a4f6ffd ("ruff check fixes") touches 11 files; 10 have nothing to do with expenses —
credit_sales.py, invoices.py, offers.py, partial_payments.py (36 lines),
pos_profile.py, shifts.py, install.py, overrides/pricing_rule.py,
test_packed_items_regression.py, test_promotions.py. Dict→dict, str(e)→{e!s},
import reordering, in modules several other open PRs own. N4 asked for one file to be
import-sorted; the fix brought ten passengers.

The churn itself is safe — I imported the eight non-test modules under site context and each
loads clean, and both except … as e → except … rewrites (partial_payments.py:319,
:469) have bodies that use frappe.get_traceback(), not e.

One of them is a real bug fix and should not ship as formatting: pos_profile.py:7 adds
from frappe.utils import cint. Ruff does not add imports — that is a hand fix for the F821
it flagged, and cint is genuinely undefined at pos_profile.py:155 on develop. A live
NameError in allow_credit_sale resolution, unrelated to expenses, currently shipping only
through this PR.

Done when: those commits land through their own PRs — or the PR description says they are
deliberately bundled and names who reviewed them. If the sweep lands separately, call the
cint fix out wherever it goes.

Notes, not blockers

• The one remaining ruff error is not yours. pos_next/api/invoices.py:1669 UP038 is on
  develop too — running ruff against d02f852:pos_next/api/invoices.py reports 3 errors
  there vs 1 now. It only appears in the changed-file list because the sweep pulled
  invoices.py into this diff. N4 itself is closed — test_pos_closing_shift.py:24 is clean.
• D2: 640533d adds company_currency to bootstrap.py's result["pos_profile"] — the
  dict flagged as Feature/thin develop split #360's collision point. Merge order is already agreed (318 first), so nothing
  changes; the keep-both table just understates the surface by one key now.
• bench --site … execute frappe.modules.utils.sync_customizations fails on develop too:
  POS Closing Shift: Options must be a valid DocType for field Bank Deposit —
  custom/pos_closing_shift.json:13 points at "Bank Deposits". Pre-existing at d02f852,
  same family as the test_bank_deposits.py:4 → IntegrationTestCase collection failure that
  still blocks run-tests --app pos_next. Not this PR, but nobody owns it.
• ensure_pos_expense_fixtures() commits (:147) so tearDown does not roll the fixtures
  away — deliberate, and test_promotions.py:530 already does the same. Worth knowing: on a
  site with no _Test Company it creates one (India / INR, :17-38) and commits it uncleaned.
  Reachable only from the two setUpClass methods, so running the expense tests is the only
  trigger.

What came back well

• N1 was fixed the larger way. The review offered "delete them or repoint them"; you deleted
  both rungs and the now-unused pos_profile parameter, and left a docstring saying why those
  fallbacks could never have fired. The dead LIMIT 1-without-ORDER BY went with them. That
  version does not grow back.
• N2 was solved at the right altitude. The obvious fix was four inlined get_doc calls per
  setUpClass. Instead the fixtures went into one module that reuses the promotions helpers —
  same profile naming, same _resolve_* functions — so the two suites cannot drift. Proven here,
  not assumed: this bench had only Wind Power LLC (abbr WP), no _Test Company and no _TC,
  before the run — and _Test Company existed after it. The fixture succeeds on the exact insert
  path that defeated ERPNext's own Company test record last time.
• The test count moved for the right reason. 26 → 27, with the four previously-erroring tests
  now green: the money path and the reconciliation seam are demonstrated, not claimed.

@engahmed1190
Cleaning this up means dropping the unrelated riders from #318 (offline batch/serial → #359, EOD/translations → #275, collected totals already on develop via #312, plus the a4f6ffd ruff sweep except the real cint fix in pos_profile.py, which will go in its own small PR).

That will look like a large force-push / history rewrite on PN-65-Excepenses-In-Pos-Screen: many files leave the diff, commit SHAs change, and GitHub may re-request review. The expense feature itself is not being redesigned — the PR is just being narrowed to expenses-only.

Happy to proceed once you’re OK with that churn, or we can document deliberate bundling in the PR description instead if you prefer not to rewrite.

@MohamedAliSmk — thanks for laying out the options rather than guessing. Two things: N7 is confirmed closed, and the answer on N3 is please proceed with the narrowing force-push.

N7 — closed at b8dc9a3

const companyCurrency = computed(
    () => shiftState.value.company?.default_currency || profileCurrency.value,
);

Verified across every writer of shiftState, not just the happy path:

shiftState was already imported at posShift.js:1, so the fix needed no new wiring. The
typed amount, the shift limit and the JE booking now share one documented basis end to end.
N6 is closed.

Two cosmetic leftovers, noted for completeness only — neither is worth a commit on its own,
fold them in if the branch is being rewritten anyway:

• bootstrap.py:89-99's company_currency key is now dead. getPreloadedPOSProfile()
  (stores/bootstrap.js:103) is exported but never called, so nothing consumes it.
• ExpenseDialog.vue:214's comment still reads "prop/bootstrap next"; the prop now comes
  from the store via shiftState, not from bootstrap.

N3 — proceed with the split

Your mapping is right, and one part of it is stronger than the review credited. I checked
the claim about collected totals:

$ git log --oneline develop --grep="collected" -i
48ce436 fix: track collected money alongside invoiced totals, not instead of them

f5ae353 on this branch carries the identical commit subject. That rider is not merely
misplaced — it is already on develop via #312. Keeping it guarantees a conflict for whoever
rebases next; that is a correctness argument, not a tidiness one.

Three reasons the churn is worth paying now rather than later:

1. f5ae353 is a verified duplicate of develop. It cannot merge cleanly twice.
2. Three of feat: enhance offline batch and serial number management #359's four files are byte-identical to that PR's head. Landing them through
   Pn 65 expenses in pos screen #318 means feat: enhance offline batch and serial number management #359's own review no longer governs code that ships — the reviewer there
   approved a diff that will have already merged elsewhere.
3. The cost is at its minimum today. Pn 65 expenses in pos screen #318 is 0 behind develop, the merge order with
   Feature/thin develop split #360 is already agreed (318 first), and no reviewer has an open thread mid-flight. Every
   day this waits, and certainly once Feature/thin develop split #360 rebases, the rewrite gets more expensive.

A force-push that drops files and rewrites SHAs is expected and fine here. Re-requested review
is the intended cost, not an accident.

Your plan needs no amendment. Restating it as the acceptance condition:

• 8231e26 → feat: enhance offline batch and serial number management #359
• 05a7b28 / 59acd2f / 9e9c26f → Feat/eod shift report print #275
• f5ae353 → dropped (already on develop via fix: shift closing total reflects money collected, not invoiced #312)
• a4f6ffd → dropped, except the cint import at pos_profile.py:7

On that last point — splitting the cint fix into its own small PR is the right call and worth
saying why out loud: cint is genuinely undefined at pos_profile.py:155 on develop, so that
one line closes a live NameError in allow_credit_sale resolution. Buried in a commit titled
"ruff check fixes" it reads as formatting and nobody backports it. On its own it is reviewable
in thirty seconds.

Done when: git diff --stat $(git merge-base HEAD develop)..HEAD contains only
expense-related files, and the four riders are open (or already merged) under their own numbers.

Once that lands, this is a merge. Everything else is closed and the gate is green —
27/27 on api.test_expenses, 11/11 on pos_closing_shift, 5/5 on pos_closing_print, and the
single remaining ruff error (invoices.py:1669 UP038) is pre-existing on develop.

Post-split review — head 13a653e

N3 is closed. The split is clean and the gate is green. Two mechanical items stand between
this and merge; neither is a code defect in the feature.

Verified against the rewritten history, by diffing the pre- and post-rewrite trees rather than
reading commit subjects.

The split — verified

44 files → 27. Every rider is gone:

The 20 commits remaining between d02f852 and 13a653e are all expense work.

On f5ae353: dropping it changed the tree by zero bytes — ShiftClosingDialog.vue and
pos_closing_shift.py are byte-identical before and after the rewrite. That confirms your read
that it was already on develop via #312: its unique contribution had been reconciled away by
the earlier dedup at 7818730. develop and this branch each carry exactly one
__("Collected") card; there was never a double-render risk, and there is none now.

Nothing was lost in the rewrite. The only differences between the old head b8dc9a3 and
13a653e, outside the removed riders, are the three you intended: the dead company_currency
key out of bootstrap.py, the ExpenseDialog.vue:214 comment, and the offers.py typing import
restored with the sweep.

13a653e's cleanup is safe — nothing consumed bootstrap's key. getPreloadedPOSProfile()
(stores/bootstrap.js:103) is exported but has no callers, and the dialog resolves currency from
dialogDataResource and shiftState.company.default_currency, neither of which touches it.

Gate, on a live bench

Frappe 15.111.1 + ERPNext 15.111.0, posnext.local, MariaDB.

N6/N7 from 6c80f31 survived the rewrite intact, and both suites still pass on a branch whose
SHAs all changed.

Two things left

1. N4 regressed · test_pos_closing_shift.py:11

pos_next/pos_next/doctype/pos_closing_shift/test_pos_closing_shift.py:11:1: I001 [*] Import block is un-sorted or un-formatted

Dropping a4f6ffd wholesale took its one legitimate line with it — the blank line after the
import block in test_pos_closing_shift.py was the only part of that sweep that belonged to this
PR. Understandable casualty of a clean revert; it just needs putting back.

Done when: ruff check --fix pos_next/pos_next/doctype/pos_closing_shift/test_pos_closing_shift.py.

Worth noting the upside: invoices.py left the diff with the sweep, so the pre-existing UP038
that had to be caveated last round is out of scope. Once this one line is back, the changed
files are ruff-clean outright for the first time on this branch — no asterisk.

2. Rebase · 2 behind

develop moved to 582ef7b when your own #373 merged at 15:44. 13a653e is 2 behind. D1 asked
for current, so this needs a rebase before merge — trivial, and no conflict surface: #373 touches
pos_profile.py, which this branch no longer carries.

Then it merges

Everything else is closed and verified: B1 H1–H4, M1–M7, T1–T4, L1–L3, D1–D2, and N1–N7. The
security work, the lock, the currency basis, the fixtures and the reconciliation seam all hold
up under a green run.

The split was the right call and it was executed cleanly — the riders are gone, nothing in the
feature was disturbed, and #373 landing separately means that NameError fix reaches develop
on its own timeline instead of riding an expenses PR. Fix the one I001, rebase, and this is a
merge from my side.